Skip to content

Refactor duplicated code from ASTBase node AliasThis into a mixin template#12796

Closed
RazvanN7 wants to merge 1 commit intodlang:masterfrom
RazvanN7:Dupl_dsymbol
Closed

Refactor duplicated code from ASTBase node AliasThis into a mixin template#12796
RazvanN7 wants to merge 1 commit intodlang:masterfrom
RazvanN7:Dupl_dsymbol

Conversation

@RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Jul 1, 2021

This is mostly a proposal of getting rid of the duplication in ASTBase. If I get a green light on this, I plan to apply it for all the nodes in ASTBase.

cc @WalterBright

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12796"

@UplinkCoder
Copy link
Member

This makes it hard to see what's going on.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Jul 1, 2021

@UplinkCoder What are you referring to exactly?

If you are referring to the fields and methods of the AST nodes, I agree that it is inconvenient that you need to check astmembers to see what's going on, but I would argue that it's a better alternative than having duplicated code. Also, our testing pipeline should still catch any situations where a field/method is defined in the AST node definition of ASTCodegen, when in fact it should be defined in the mixin template.

@UplinkCoder
Copy link
Member

The constructor is suddenly not inspectable anymore.
Yes the code is boilerplate and it can be copy pasted.
But hidining it in a mixin template doesn't make it better.
also that hides the method from __traits(allMembers).

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Jul 1, 2021

ASTBase has been a constant pain and is already out of sync with the nodes in ASTCodegen. Having the shared code in a single place is worth the extra effort to inspect a different file.

also that hides the method from __traits(allMembers).

Code that does this should be updated to recursively check for mixin templates anyway.

@UplinkCoder
Copy link
Member

astbase should just be removed alltogether.
it serves no purpose.

super(loc, null); // it's anonymous (no identifier)
this.ident = ident;
}
mixin parseTimePropertiesAliasThis;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really address the duplication though, does it? There are still two copies of AliasThis present, and every time a new AST node is introduced it must still be defined twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way we could eliminate the duplication would be to have the ASTCodegen classes inherit from the ASTBase classes, however, that would require multiple inheritance since a node must inherit its parse time counterpart but also its logical ancestor (for example AddExp would need to inherit ParseTimeAddExp and BinaryExp). This could technically be done by using inheritance and alias this, but I would rather not go there since this currently isn't defined behavior (and ideally alias this would be deprecated for classes).

Instead, this PR limits the scope of the duplication. Indeed, new AST node declarations still need to be duplicated, but I would argue that this doesn't happen very often.

I couldn't come up with a solution that doesn't require to refactor the compiler entirely (e.g. stripping all AST nodes of their functions, basically transforming them into data containers) . If anyone has any propositions, I'm open to suggestions. However, I doubt that a feasible solution to get rid of the duplicated AST node declarations is possible at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't come up with a solution that doesn't require to refactor the compiler entirely (e.g. stripping all AST nodes of their functions, basically transforming them into data containers) . If anyone has any propositions, I'm open to suggestions. However, I doubt that a feasible solution to get rid of the duplicated AST node declarations is possible at this point.

Why not? We've already removed the semantic, semantic2, semantic3, resolve, toIR, toCtype, toElem, toDt, and probably many more this way.

  1. There are pointless member functions:

    dmd/src/dmd/statement.d

    Lines 216 to 249 in 0ff19fb

    final bool usesEH()
    {
    extern (C++) final class UsesEH : StoppableVisitor
    {
    alias visit = typeof(super).visit;
    public:
    override void visit(Statement s)
    {
    }
    override void visit(TryCatchStatement s)
    {
    stop = true;
    }
    override void visit(TryFinallyStatement s)
    {
    stop = true;
    }
    override void visit(ScopeGuardStatement s)
    {
    stop = true;
    }
    override void visit(SynchronizedStatement s)
    {
    stop = true;
    }
    }
    scope UsesEH ueh = new UsesEH();
    return walkPostorder(this, ueh);
    }

  2. There doesn't need to be a big clear-out done, just anything that isn't to do with examining itself. e.g. from Statement:

    dmd/src/dmd/statement.h

    Lines 119 to 129 in 0ff19fb

    virtual Statement *getRelatedLabeled() { return this; }
    virtual bool hasBreak() const;
    virtual bool hasContinue() const;
    bool usesEH();
    bool comeFrom();
    bool hasCode();
    virtual Statement *scopeCode(Scope *sc, Statement **sentry, Statement **sexit, Statement **sfinally);
    virtual Statements *flatten(Scope *sc);
    virtual Statement *last();
    virtual ReturnStatement *endsWithReturnStatement() { return NULL; }

    Remove the above highlighted functions, and astbase can just publicly import dmd.statement...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's not that easy. Looking at statement.d it imports a lot of modules that don't have anything to do with parsing. Before being able to import statement.d we need to get rid of all of those dependencies. That is no easy task and it requires a lot of refactorings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be surprised if it stays that way after dealing with the above functions.

Copy link
Contributor Author

@RazvanN7 RazvanN7 Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that statement.d has many dependencies. Even if we move out everything that relies on semantic information from statement.d, we will still have some Expression fields. expression.d has other dependencies that rely on semantic information, so now we need to the same for it too etc. My point is that, before we can simply do import statement; in ASTBase, we need to do this cleanup for all the AST nodes, otherwise the import dependencies are going to drag semantic information into ASTBase.

@kinke
Copy link
Contributor

kinke commented Jul 1, 2021

astbase should just be removed alltogether.
it serves no purpose.

I've never understood what it's for. Is something actually using this file at all? We exclude it for LDC, so this refactoring would make the LDC frontend actually look much uglier than it has to.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Jul 1, 2021

@kinke It was initially implemented to be able to separate the parser from the semantic analysis, so that users of dmd as a lib have the capability of building and extending solely the parser. I don't know if any projects are actually using it.

@benjones
Copy link
Contributor

benjones commented Jul 1, 2021

@kinke It was initially implemented to be able to separate the parser from the semantic analysis, so that users of dmd as a lib have the capability of building and extending solely the parser. I don't know if any projects are actually using it.

Looks like a few projects use it? https://github.com/search?q=import+dmd.astbase&type=code

@kinke
Copy link
Contributor

kinke commented Jul 1, 2021

Thx Razvan and Ben; after quickly glancing at the D-filtered results, it looks like 2 projects remain - https://github.com/dragospetrescu/SHDP-Parser/blob/5f01aaff66fd0d632fce421a7378a6fc138283e6/source/worker.d and https://github.com/DmitryOlshansky/dlang-graal/blob/27c672d74124530e9abef6be5d204601c3ecd0cc/dtool.d, besides some DMD dub package tests like https://github.com/dlang/dmd/blob/master/test/dub_package/avg.d.

Edit: And these 2 projects seem very incomplete/abandoned.

@Geod24
Copy link
Member

Geod24 commented Jul 2, 2021

@kinke It was initially implemented to be able to separate the parser from the semantic analysis, so that users of dmd as a lib have the capability of building and extending solely the parser. I don't know if any projects are actually using it.

It's the first instance I can recall of designing a library for the sake of it. There's already libdparse, and associated libraries, if one wants to just parse D code.

And for the record, using mixins to remove duplication is like painting on a rusty car.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Jul 2, 2021

I'm trying to find a fix here to remove the duplication in ASTBase, but it looks like every direction I take encounters resistance. I suggest we start a discussion on what the best path is and stick with it, otherwise, this duplication will continue to exist.

We have 3 options:

  1. Pull out everything that has something to do with semantic from AST nodes. That way, we end up with classes that simply contain some fields and methods for self examination. This is what I think to be the best option, however, when we tried it [1], it was rejected.

  2. The mixin hack that this PR currently employs. I agree that this is just a workaround, but at least it removes the duplication of methods and fields, which IMHO is a step forward because we get rid of the inconsistencies that have already piled up. However, this option is also rejected.

  3. Completely remove ASTBase. I agree that things are not ideal right now, but what ASTBase enables is building the compiler without any semantic dependencies. This is a step forward for dmd compiler as a library; a small step, but nevertheless a step forward. I don't see this as a viable alternative and if a PR were made to do so, that would be rejected also.

If there are any other alternatives, I am open to discussions, but one thing is certain: we need to get rid of this duplication. I would very much appreciate it if we have a debate on what is the best course of action, rather then just turn down any attempt at fixing the situation.

[1] #11788

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Jul 2, 2021

It's the first instance I can recall of designing a library for the sake of it. There's already libdparse, and associated libraries, if one wants to just parse D code.

Yes, libdparse which is continually left behind by new frontend additions. Just take a look at what's happening right now: lidbparse has a memory corruption bug and we cannot update dscanner to unclogg the phobos PR queue. This would not have happened if dmd as a lib was used.

@kinke
Copy link
Contributor

kinke commented Jul 2, 2021

I know of 3 major projects using DMD as a library - GDC, LDC and Visual D, and none of them use ASTBase. So I'm clearly for option 3, removing it. Edit: Short-term, that is.

@WalterBright
Copy link
Member

I'd like to explore something along the lines of an abstract interface, as that's the more traditional way of doing it rather than boilerplate insertion. The interface can use static polymorphism (aka template) or dynamic polmorphism (aka abstract class).

@WalterBright
Copy link
Member

The abstract interface should not have any data members.

@RazvanN7 RazvanN7 closed this Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

Comments